Name the writing release in the storage-format upgrade refusal#315
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84449856f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Address P2 review feedback on #315: - crossversion_upgrade.rs: OMNIGRAPH_OLD_BIN set-but-missing now fails loudly instead of skipping vacuously (only an UNSET var is a legitimate skip) — a misconfigured CI install path no longer passes without minting a v3 graph. - crossversion_upgrade.rs: tighten the refusal assertion from `0.7` to the full `0.6.2 to 0.7.2` range, matching its in-source sibling. - ci.yml: add db/manifest/tests.rs to the run_upgrade_ci path filter so a future edit to the in-source refusal assertion still triggers the cross-version job. - migrations.rs: reword the unreachable release fallback to read naturally after "created by omnigraph " (defensive only; 1-3 are mapped, >=CURRENT is caught by the ceiling guard first). Declined: adding a v4 row to the upgrade.md stamp->release table — that table is for sub-CURRENT (refused) stamps only, and a v4 graph under a v4 binary is never refused, so a v4 row would wrongly imply a current graph needs rebuilding.
|
Thanks for the review — addressed in
Declined: the v4 row in the |
|
Rebased onto current main (post Lance 9.0.0-beta.15) as a clean re-apply — the branch's merge-commit history made a plain rebase impractical, so this is the PR's net diff on a fresh base with authorship preserved. Conflicts were testing.md row noise; one substantive touch-up: the CI install comment claimed 0.7.2 shares Lance 7.0.0 with current, which the Lance 9 migration made stale — reworded, and notably the cross-version test now doubles as a cross-substrate check (Lance-9 binary reading a Lance-7-written manifest to find the stamp). Validated locally against a genuine crates.io 0.7.2 binary: the v3 mint → refusal-with-named-release → rebuild round-trip passes. Full workspace gate green. |
e79b1cb to
99e49f8
Compare
|
Per review discussion: the CI job is removed entirely — ci.yml is now byte-identical to main. The cross-version test stays, gated on |
99e49f8 to
c6a3b2e
Compare
…and) The storage-format refusal now names the release line that wrote each internal-schema stamp (e.g. v3 -> 0.6.2-0.7.2) plus the exact export/init/load commands — fail-closed but self-service; the message is engine-shared so server boot-quarantine and cluster status get it free. Adds the OMNIGRAPH_OLD_BIN-gated cross-version test: mint a GENUINE v3 graph with a real omnigraph-cli 0.7.2 binary, assert the current binary refuses it with the release-named message, and the documented rebuild round-trips. Deliberately NOT wired into CI (the write_cost_s3 disposition): compiling 0.7.2 from source is a 15-25 min job and the stamp seam changes a few times a year — testing.md documents the on-demand invocation instead. Rebased onto current main across the Lance 9.0.0-beta.15 migration; validated locally against a genuine 0.7.2 binary — the v3 round-trip passes on the Lance-9 main, which also makes it a live cross-substrate read-compat check (a Lance-9 binary reading a Lance-7-written manifest to find the stamp).
c6a3b2e to
e59fea8
Compare
What & why
The storage-format upgrade refusal told operators to export with "the older omnigraph binary that created it" without saying which release — leaving them to guess, and stuck if the fleet was already upgraded. This names the exact release line that wrote each internal-schema stamp (e.g. v3 → 0.6.2–0.7.2) plus the precise
export/init/loadcommands, keeping the strand-model upgrade fail-closed but self-service. The message is engine-shared, so server boot-quarantine logs andcluster statusobservations get the better text for free.Backing issue / RFC
Checklist
upgrade.md(release-named message + stamp→release table),versioning.md,testing.mdNotes for reviewers
crates/omnigraph-cli/tests/crossversion_upgrade.rsmints a real v3 graph with omnigraph 0.7.2 and is gated onOMNIGRAPH_OLD_BIN; it skips in the default suite and runs in the new gatedcrossversion_upgradeCI job (installsomnigraph-cli@0.7.2). It is the empirical proof the existing stamp-rewind test can't give.Note
Low Risk
Messaging and documentation only on an already fail-closed open path; no change to migration policy or in-place upgrades. Residual risk is limited to incorrect stamp→release mapping in operator-facing text.
Overview
When a graph’s internal-schema stamp is below what the current binary serves, the refusal message is more actionable: it names the release line that wrote that stamp (via new
release_for_internal_schema_versioninmigrations.rs) and spells out the fullexport→init→loadrebuild steps instead of telling operators to use “the older binary” with no version hint. Open logic and the strict single-version strand model are unchanged—still fail-closed.Tests and CI: In-source checks assert the named release appears in refusals;
crossversion_upgrade.rs(gated onOMNIGRAPH_OLD_BIN) builds a genuine v3 graph with omnigraph 0.7.2 and verifies refusal text plus export round-trip including vectors. A newcrossversion_upgradejob andrun_upgrade_cipath filter run that test on PRs that touch the upgrade surface.Docs:
upgrade.md,versioning.md, andtesting.mddocument the new message, stamp→release table, and how to run the gated test locally and in CI.Reviewed by Cursor Bugbot for commit e79b1cb. Bugbot is set up for automated code reviews on this repo. Configure here.
Greptile Summary
This PR improves the operator experience for the storage-format upgrade path by making the sub-CURRENT refusal message self-service: it now names the exact release line that wrote each internal-schema stamp (via
release_for_internal_schema_versioninmigrations.rs) and prints the fullexport→init→loadcommand sequence. No behavioral change to the strand model or open-path logic.migrations.rs): a newrelease_for_internal_schema_version(stamp)function maps v1–v4 stamps to release strings;refuse_if_stamp_unsupportedembeds this in the sub-CURRENT error so operators know which binary to fetch.tests.rsasserts the named release appears in the refusal;crossversion_upgrade.rs(gated onOMNIGRAPH_OLD_BIN) mints a genuine v3 graph with the 0.7.2 binary and verifies the refusal text plus a full round-trip including aVectorcolumn.upgrade.mdadds the stamp→release table (now covering v1–v4), reproduces the updated error message verbatim, and documentsversioning.mdandtesting.mdto reflect the new function and CI job.Confidence Score: 5/5
Safe to merge — the change is limited to the text of an existing error message and its surrounding documentation; the refusal logic itself is untouched and still fail-closed.
The only code path changed is the string returned by the new
release_for_internal_schema_versionfunction and its embedding in the error message. The open-path guard, stamp comparison, and MIN_SUPPORTED == CURRENT invariant are all unchanged. Tests cover the new assertion both in-source (synthetic stamp rewind) and via a real cross-version binary.The
4 => "0.8.x"arm inmigrations.rsdiffers from the "0.8.x and later" wording inupgrade.md— a minor drift that only matters once a future release maintains v4 without updating the string.Important Files Changed
release_for_internal_schema_versionmapping stamps to release strings; updatesrefuse_if_stamp_unsupportedto embed the release name in the error. The_fallback is reworded to "an unrecognized older release" (addressing prior review) and is unreachable in practice. The v4 entry maps to "0.8.x" — consistent with the current binary but slightly narrower than the docs' "0.8.x and later" phrasing.release_names_the_writing_line_for_each_stamp(unit) and extendssub_current_graph_is_refused_then_rebuilt_via_export_importto assert both "0.7.2" and "export" appear in the refusal message. The new assertions are well-targeted and match the exact message template.release_for_internal_schema_versionby name and documents the fail-closed + self-service property of the new refusal message; no structural changes to the versioning policy.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Omnigraph::open / open_read_only] --> B[read_stamp from __manifest] B --> C{stamp vs CURRENT} C -->|stamp > CURRENT| D["Error: upgrade omnigraph\nbefore opening this graph"] C -->|stamp == CURRENT| E[Open succeeds] C -->|stamp < MIN_SUPPORTED| F[release_for_internal_schema_version stamp] F --> G{Match stamp} G -->|1| H["0.3.1 or earlier"] G -->|2| I["0.4.1 to 0.6.1"] G -->|3| J["0.6.2 to 0.7.2"] G -->|4| K["0.8.x"] G -->|_| L["an unrecognized older release"] H & I & J & K & L --> M["Error: __manifest stamped at v{stamp}...\nThis graph was created by omnigraph {release}.\nRebuild: export → init → load"]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A[Omnigraph::open / open_read_only] --> B[read_stamp from __manifest] B --> C{stamp vs CURRENT} C -->|stamp > CURRENT| D["Error: upgrade omnigraph\nbefore opening this graph"] C -->|stamp == CURRENT| E[Open succeeds] C -->|stamp < MIN_SUPPORTED| F[release_for_internal_schema_version stamp] F --> G{Match stamp} G -->|1| H["0.3.1 or earlier"] G -->|2| I["0.4.1 to 0.6.1"] G -->|3| J["0.6.2 to 0.7.2"] G -->|4| K["0.8.x"] G -->|_| L["an unrecognized older release"] H & I & J & K & L --> M["Error: __manifest stamped at v{stamp}...\nThis graph was created by omnigraph {release}.\nRebuild: export → init → load"]Reviews (6): Last reviewed commit: "docs: self-service upgrade message + gat..." | Re-trigger Greptile